Refactor Protocol Handling and Update Dependencies#136
Conversation
- Replaced `ProtocolExtractor` with a new `ProtocolParser` for improved protocol extraction and handling. - Updated the `Erc721EthscriptionsCollectionParser` to include `merkle_root` in various operations, enhancing metadata management. - Removed the deprecated `GenericProtocolExtractor` and its associated tests to streamline the codebase. - Updated dependencies in `Gemfile.lock` to ensure compatibility with the latest versions. - Enhanced integration tests to validate the new parser functionality and ensure proper handling of collection operations.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the protocol handling system by removing the generic protocol extractor, making the merkle_root field required in collection operations, and renaming core classes from "extractor" to "parser" for consistency.
Key Changes:
- Removed
GenericProtocolExtractorand all related test files, limiting protocol support to only ERC-20 tokens and ERC-721 collections - Made
merkle_roota required field (bytes32) increate_collectionandedit_collectionoperations - Renamed
ProtocolExtractortoProtocolParserand updated all references throughout the codebase
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| app/models/protocol_parser.rb | Renamed from ProtocolExtractor to ProtocolParser; removed generic protocol support and associated methods |
| app/models/generic_protocol_extractor.rb | Completely removed - generic protocol extraction no longer supported |
| app/models/erc721_ethscriptions_collection_parser.rb | Updated schemas to include required merkle_root field; updated method signatures to remove item_id parameter |
| app/models/ethscription_transaction.rb | Updated reference from ProtocolExtractor to ProtocolParser |
| spec/models/protocol_extractor_spec.rb | Removed - tests for generic protocol functionality no longer needed |
| spec/models/generic_protocol_extractor_spec.rb | Removed - tests for generic protocol extractor |
| spec/models/generic_protocol_extractor_type_hints_spec.rb | Removed - tests for type hints in generic protocol |
| spec/models/generic_protocol_extractor_collections_spec.rb | Removed - tests for collections via generic protocol |
| spec/models/erc721_ethscriptions_collection_parser_spec.rb | Updated all test cases to include merkle_root field |
| spec/models/erc721_collections_import_fallback_spec.rb | Added merkle_root to test fixtures; fixed indentation issues |
| spec/integration/token_protocol_e2e_spec.rb | Updated test to reflect that whitespace in JSON now causes rejection; updated to use ProtocolParser |
| spec/integration/collections_protocol_spec.rb | Added merkle_root to all collection creation tests; improved JSON validation in assertions |
| spec/integration/collections_protocol_e2e_spec.rb | Added merkle_root to test fixtures; updated to use ProtocolParser |
| Gemfile.lock | Updated dependency versions including base64, benchmark, bigdecimal, connection_pool, drb, i18n, minitest, timeout, and uri; removed ruby2_keywords (transitive dependency) |
Comments suppressed due to low confidence (2)
app/models/protocol_parser.rb:6
- The reference on line 6 to
GENERIC_DEFAULT_PARAMSwas removed when the generic protocol extractor was removed, but line 5 still referencesCOLLECTIONS_DEFAULT_PARAMSwhich is used. This is fine, but the removal of the generic protocol functionality means this parser now only supports two specific protocol types (tokens and collections), which is a significant functional change that should be well-documented in the PR description.
app/models/protocol_parser.rb:16 - The comment on line 16 states "Try collections parser next (if enabled)" but there's no longer any conditional logic or environment variable check for enabling/disabling the collections parser. This comment is outdated and should be updated to simply say "Try collections parser next" to match the actual code behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'twitter_link', twitter_link, | ||
| 'discord_link', discord_link | ||
| ] | ||
| merkle_root = meta.fetch('merkle_root') |
There was a problem hiding this comment.
The build_metadata_object method uses meta.fetch('merkle_root') which will raise a KeyError if the key is not present. This is inconsistent with the import fallback logic on line 187 which uses raw_metadata['merkle_root'] || ZERO_HEX_BYTES32 to provide a default.
Consider changing line 283 to:
merkle_root = meta.fetch('merkle_root', ZERO_HEX_BYTES32)This would make the method more defensive and consistent with how merkle_root is handled elsewhere in the import path.
| merkle_root = meta.fetch('merkle_root') | |
| merkle_root = meta.fetch('merkle_root', ZERO_HEX_BYTES32) |
| 'discord_link' => '', | ||
| 'background_color' => '#FFFFFF', | ||
| 'total_supply' => 2, | ||
| 'merkle_root' => zero_merkle_root |
There was a problem hiding this comment.
The indentation is inconsistent in the collections_json hash. Lines 19-22 have different indentation (8 spaces instead of 10) compared to lines 12-18. This should be corrected for consistency:
'discord_link' => '',
'background_color' => '#FFFFFF',
'total_supply' => 2,
'merkle_root' => zero_merkle_rootShould be indented to align with the other keys in the hash.
| 'discord_link' => '', | |
| 'background_color' => '#FFFFFF', | |
| 'total_supply' => 2, | |
| 'merkle_root' => zero_merkle_root | |
| 'discord_link' => '', | |
| 'background_color' => '#FFFFFF', | |
| 'total_supply' => 2, | |
| 'merkle_root' => zero_merkle_root |
| 'create_collection_and_add_self' => { | ||
| keys: %w[metadata item], | ||
| # ((CollectionParams),(ItemData)) - ItemData without ethscription_id (item refers to itself) | ||
| # ((CollectionParams),(ItemData)) - ItemData mirrors ItemData struct (no ethscriptionId field) |
There was a problem hiding this comment.
The comment on line 31 states "ItemData mirrors ItemData struct (no ethscriptionId field)" which is redundant - it says ItemData mirrors ItemData. This should be clarified to explain what the ItemData struct mirrors or what distinguishes it from other item representations.
Consider revising to something like: "ItemData struct contains item fields without the ethscriptionId (self-referencing)"
| # ((CollectionParams),(ItemData)) - ItemData mirrors ItemData struct (no ethscriptionId field) | |
| # ((CollectionParams),(ItemData)) - ItemData struct contains item fields without the ethscriptionId (self-referencing) |
| ) | ||
| ) do |results| | ||
| # The protocol extractor should preserve field order | ||
| # The protocol parser should preserve field order |
There was a problem hiding this comment.
The comment on line 290 has been updated to refer to "protocol parser" (previously "protocol extractor"). However, the rest of the comment might need review for consistency - it mentions "should preserve field order" which is good, but consider adding context about what parser is being referenced if it's not clear from context.
| # The protocol parser should preserve field order | |
| # The on-chain protocol parser in the Ethscriptions contract should preserve field order | |
| # to ensure correct decoding of Solidity structs. |
| @@ -219,4 +221,3 @@ | |||
| end | |||
| end | |||
| end | |||
There was a problem hiding this comment.
The trailing empty line at the end of the file was removed. While this is a minor formatting change, it's a common convention to have a newline at the end of files. Consider keeping the trailing newline for consistency with common style guides.
| # Check if protocol was extracted | ||
| begin | ||
| protocol, operation, encoded_data = ProtocolExtractor.for_calldata(data_uri) | ||
| protocol, operation, encoded_data = ProtocolParser.for_calldata(data_uri) |
There was a problem hiding this comment.
This assignment to encoded_data is useless, since its value is never read.
| protocol, operation, encoded_data = ProtocolParser.for_calldata(data_uri) | |
| protocol, operation = ProtocolParser.for_calldata(data_uri) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…0FixedDenominationParser - Added `structured_params` method to convert operation parameters into a hash format for downstream services. - Introduced `encode_calldata` method to encode parameters into the ABI tuple required by the manager contract. - Updated `ProtocolParser` to utilize the new methods for improved parameter handling and encoding.
ProtocolExtractorwith a newProtocolParserfor improved protocol extraction and handling.Erc721EthscriptionsCollectionParserto includemerkle_rootin various operations, enhancing metadata management.GenericProtocolExtractorand its associated tests to streamline the codebase.Gemfile.lockto ensure compatibility with the latest versions.Note
Introduce ProtocolParser, extend ERC-721 collection parsing with mandatory merkle_root, drop GenericProtocolExtractor, and update tests and deps.
ProtocolExtractorwith newProtocolParser(app/models/protocol_parser.rb).GenericProtocolExtractorandProtocolExtractoralong with their specs.EthscriptionTransactionto useProtocolParser.for_calldata.app/models/erc721_ethscriptions_collection_parser.rb):merkle_rootincreate_collectionandedit_collectionops and in combined create ops.merkle_rootdefaulting to zero when missing; refactor ID handling and item building; enforce itemmerkle_proofpresence.app/models/erc20_fixed_denomination_parser.rb):structured_paramsandencode_calldatahelpers for downstream/ABI use.ProtocolParserand includemerkle_rootin payloads.Gemfile.lockand updatefacet_rails_commonrevision.Written by Cursor Bugbot for commit a908067. This will update automatically on new commits. Configure here.